Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts DDL statement ordering and expression normalization to prevent invalid diffs / execution failures when domains (and their constraints) reference functions.
Changes:
- Updates CREATE statement ordering to defer domains that reference newly-added functions until after functions are created.
- Extends IR normalization to strip same-schema function qualifiers from domain CHECK constraint expressions to avoid perpetual diffs.
- Adds a new
testdata/difffixture covering domain CHECK constraints that call a function.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/diff/diff.go |
Adds domain→function dependency detection and defers function-dependent domain creation until after functions are created. |
ir/normalize.go |
Normalizes domain constraints using the domain schema to strip same-schema function qualifiers in CHECK expressions. |
testdata/diff/create_domain/domain_function_check_dependency/plan.txt |
New golden output for human plan format for the domain/function dependency case. |
testdata/diff/create_domain/domain_function_check_dependency/plan.sql |
New golden output for SQL plan format for the domain/function dependency case. |
testdata/diff/create_domain/domain_function_check_dependency/plan.json |
New golden output for JSON plan format for the domain/function dependency case. |
testdata/diff/create_domain/domain_function_check_dependency/old.sql |
New test “old” schema for the domain/function dependency scenario. |
testdata/diff/create_domain/domain_function_check_dependency/new.sql |
New test “new” schema defining the function + domain CHECK constraint. |
testdata/diff/create_domain/domain_function_check_dependency/diff.sql |
New expected diff SQL output for the scenario. |
Comments suppressed due to low confidence (1)
internal/diff/diff.go:1464
- The new split between
typesWithoutFunctionDepsanddomainsWithFunctionDepscan leaveCREATE TABLEstatements that use those deferred domains in the first ("without function deps") batch. Example from #254: a table column of typecustom_iddoes not reference the validation function directly, so it will be emitted beforedomainsWithFunctionDepsare created, causingCREATE TABLEto fail because the domain type doesn't exist yet.
Consider deferring tables that reference any deferred domain type (e.g., scan table.Columns[].DataType for domain names), or alternatively emit domains first without the function-dependent CHECK constraints and ALTER DOMAIN ... ADD CONSTRAINT after functions exist (similar to deferred FK constraints).
// Separate types into domains with/without function dependencies
// Domains with function deps (e.g., CHECK constraints referencing functions) must be created after functions
typesWithoutFunctionDeps := []*ir.Type{}
domainsWithFunctionDeps := []*ir.Type{}
for _, typeObj := range d.addedTypes {
if typeObj.Kind == ir.TypeKindDomain && domainReferencesNewFunction(typeObj, newFunctionLookup) {
domainsWithFunctionDeps = append(domainsWithFunctionDeps, typeObj)
} else {
typesWithoutFunctionDeps = append(typesWithoutFunctionDeps, typeObj)
}
}
// Create types WITHOUT function dependencies (enum, composite, and domains without function deps)
generateCreateTypesSQL(typesWithoutFunctionDeps, targetSchema, collector)
// Create sequences
generateCreateSequencesSQL(d.addedSequences, targetSchema, collector)
// Build map of existing tables (tables being modified, so they already exist)
existingTables := make(map[string]bool, len(d.modifiedTables))
for _, tableDiff := range d.modifiedTables {
key := fmt.Sprintf("%s.%s", tableDiff.Table.Schema, tableDiff.Table.Name)
existingTables[key] = true
}
var shouldDeferPolicy func(*ir.RLSPolicy) bool
if len(newFunctionLookup) > 0 {
shouldDeferPolicy = func(policy *ir.RLSPolicy) bool {
return policyReferencesNewFunction(policy, newFunctionLookup)
}
}
// Separate tables into those that depend on new functions and those that don't
// This ensures we create functions before tables that use them in defaults/checks
tablesWithoutFunctionDeps := []*ir.Table{}
tablesWithFunctionDeps := []*ir.Table{}
for _, table := range d.addedTables {
if tableReferencesNewFunction(table, newFunctionLookup) {
tablesWithFunctionDeps = append(tablesWithFunctionDeps, table)
} else {
tablesWithoutFunctionDeps = append(tablesWithoutFunctionDeps, table)
}
}
// Create tables WITHOUT function dependencies first (functions may reference these)
deferredPolicies1, deferredConstraints1 := generateCreateTablesSQL(tablesWithoutFunctionDeps, targetSchema, collector, existingTables, shouldDeferPolicy)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tianzhou
left a comment
There was a problem hiding this comment.
I’d appreciate it if we could split this into several PRs. Thank you!
|
sure thing @tianzhou, will get that knocked out today |
Closes #254 and #256
Problem
@alecthomas explained the problem quite well in the issue so i won't repeat that here.
Fix
Extended the existing function dependency handling pattern to cover domains. The codebase already splits tables into those with/without function dependencies and creates them at appropriate points relative to function creation. This change applies the same approach to domains:
Additional Fix: Domain constraint expression normalization
Problem
Ran into a separate issue when running integration tests that was causing a test to fail because PostgreSQL always returns fully-qualified function names in constraint definitions (e.g., public.validate_id(VALUE)), but the source SQL uses unqualified names, so the constraint definitions didn't match on re-comparison.
Fix
strip same-schema prefixes from function calls, matching the existing pattern in
normalizePolicyExpression(added for Issue #220). This ensures consistent comparison regardless of whether the source SQL includes schema qualification.Alternative Fixes
Normalize expressions to always use fully-qualified names before comparing — would require adding schema prefixes to source expressions.
Additional Fix: SQL function body dependency ordering
Problem
PostgreSQL's
pg_dependcatalog doesn't track function-to-function references inside SQL function bodies, so the existing topological sort had no dependency data to work with and fell back to alphabetical ordering. This caused failures when a function likea_wrapper()calledz_helper()— alphabeticallya_wrapperwould be created first, but it needsz_helperto exist.Fix
Added
buildFunctionBodyDependencies()which scans function bodies using the existingfunctionCallRegexto detect function calls, then populates theDependenciesfield before the topological sort runs. This supplements thepg_dependdata with body-level references.Happy to break these fixes into separate PRs if desired.